Add performance and allocation review learnings from PR #327#354
Merged
jonathanpeppers merged 1 commit intomainfrom May 4, 2026
Merged
Add performance and allocation review learnings from PR #327#354jonathanpeppers merged 1 commit intomainfrom
jonathanpeppers merged 1 commit intomainfrom
Conversation
rmarinho
requested review from
jonathanpeppers and
simonrozsival
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the repository’s reviewer guidance and Copilot instructions to incorporate performance/allocation learnings from PR #327, aiming to reduce allocation-heavy suggestions and align reviews with established repo patterns.
Changes:
- Expanded
review-rules.mdwith new performance/allocation rules (ArrayPool usage, reusable buffers, avoiding string intermediates, async/stackalloc constraints, thread-safety documentation). - Updated
SKILL.mdworkflow guidance to grep for existing repo patterns before proposing new infrastructure. - Enhanced
.github/copilot-instructions.mdwith buffer-management and thread-safety guidance, plus netstandard2.0 notes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .github/skills/android-tools-reviewer/references/review-rules.md | Adds new performance/allocation and thread-safety review rules, plus updated netstandard2.0 guidance. |
| .github/skills/android-tools-reviewer/SKILL.md | Adds workflow guidance to prefer existing repo patterns (ArrayPool/ObjectPool/ProcessUtils) before suggesting alternatives. |
| .github/copilot-instructions.md | Updates Copilot guidance for netstandard2.0, buffer reuse/ArrayPool, and thread-safety documentation expectations. |
rmarinho
force-pushed
the
improvements/review-learnings-from-327
branch
from
d361fce to
fc4c108
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the repository’s reviewer skill guidance and Copilot instructions to incorporate performance/allocation review learnings (from PR #327), with emphasis on hot-path buffer reuse and avoiding avoidable allocations in protocol code.
Changes:
- Expanded reviewer rules to include new performance/allocation guidance (ArrayPool, reusable buffers, avoiding string intermediates, async
stackalloccaveat, loop helper reuse, thread-safety remarks). - Updated the reviewer workflow to explicitly prefer existing repo patterns before proposing new infrastructure.
- Enhanced Copilot instructions with buffer reuse guidance and additional netstandard2.0 considerations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/skills/android-tools-reviewer/references/review-rules.md | Adds performance/allocation-focused review checks and thread-safety documentation guidance. |
| .github/skills/android-tools-reviewer/SKILL.md | Adds workflow guidance to grep for existing repo patterns before suggesting new ones. |
| .github/copilot-instructions.md | Adds Copilot guidance on buffer reuse and netstandard2.0 compatibility pitfalls. |
Address review feedback: - Remove incorrect UTF-8 string literal guidance (ReadOnlySpan<byte> exists on netstandard2.0 via System.Memory) - Remove "No string intermediates in protocols" rule (overly prescriptive) - Remove "No stackalloc in async I/O" rule (compiler enforces this) - Remove Encoding.ASCII recommendation (wrong for non-ASCII content) - Move valid rules to split files (csharp-rules.md) after PR #355 split - Keep: buffer reuse, thread-safety docs, loop helper reuse, prefer existing repo patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
force-pushed
the
improvements/review-learnings-from-327
branch
from
fc4c108 to
89ab055
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds performance and allocation review learnings from PR #327 to the reviewer skill and Copilot instructions.
Changes
.github/copilot-instructions.mdArrayPool<byte>.Shared+ reusablereadonly byte[]fields)<remarks>This class is not thread-safe.</remarks>).github/skills/android-tools-reviewer/SKILL.mdArrayPool,ObjectPool,MemoryStreamPool,ProcessUtilsbefore suggesting new infrastructure.github/skills/android-tools-reviewer/references/csharp-rules.mdRemoved from original PR (per review feedback)
ReadOnlySpan<byte>works on netstandard2.0 viaSystem.MemoryEncoding.ASCII.GetBytes()recommendation — wrong for non-ASCII contentAlso rebased onto main to resolve merge conflicts from PR #355 (which split
review-rules.mdinto per-category files).